Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for HSCAN NOVALUES #2722

Merged
merged 10 commits into from
Aug 18, 2024

Conversation

atakavci
Copy link
Contributor

Closes/Fixes #2721

brings new functions to the API ;

  • IDatabase.HashScanNoValues
  • IDatabase.HashScanNoValues
  • IDatabaseAsync.HashScanNoValuesAsync

to enable the return type consisting of keys in the hash.
added some unit and integration tests in paralled to what is there for HashScan and HashScanAsnyc.

Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few changes requested here but looking awesome - with tidy we should be good, thanks for this!

src/StackExchange.Redis/RedisDatabase.cs Outdated Show resolved Hide resolved
src/StackExchange.Redis/RedisDatabase.cs Outdated Show resolved Hide resolved
tests/StackExchange.Redis.Tests/HashTests.cs Outdated Show resolved Hide resolved
tests/StackExchange.Redis.Tests/ScanTests.cs Show resolved Hide resolved
@WeihanLi

This comment was marked as outdated.

- add a new message.create
- test version guards
@atakavci
Copy link
Contributor Author

atakavci commented Jun 13, 2024

hey @WeihanLi , checking where the similar approach is applied and will try that. thanks !

where is this quote from?

This feature will be released in Redis 8.0 and is available since 7.2.5 in Valkey from my research

@WeihanLi
Copy link
Contributor

WeihanLi commented Jun 13, 2024

where is this quote from?

This feature will be released in Redis 8.0 and is available since 7.2.5 in Valkey from my research

From my research:
redis: redis/redis#12765

valkey:

double checked with valkey, not supported so far, 7.2.5 does not support this feature 😢

Supported with Redis 7.4

@atakavci
Copy link
Contributor Author

atakavci commented Jun 13, 2024

my 2 cents on this,,
making developers life easier and keeping client compatible in more situations/environments is cool.
on the other hand handling such scenarios and doing more (than actually needed/requested) under the hood, this introduces the possibility of significant increase in resource usage both on client and server side while people probably wont even get noticed about it (probably until something bad).
what do you think about it ?

@atakavci
Copy link
Contributor Author

hey @NickCraver, how about taking another look to this one?? Let me know if you have concerns or there are things to fix and i ll deal with it.

Copy link
Collaborator

@mgravell mgravell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; the unrelated whitespace changes are ... not ideal in a feature PR, but I think we should just eat them - they're fine

TODO (not necessarily as part of this PR, for timing): release notes

@NickCraver
Copy link
Collaborator

I noticed in updating this that we have a mismatch here on HashScanNoValues (2 overloads)vsHashScanNoValuesAsync` (1 method).

I'd ask when adding here: that's the purpose of the smaller overload for HashScanNoValues? It seems like we should just remove it and normalize on the one method, but maybe I'm missing a use case.

@atakavci
Copy link
Contributor Author

atakavci commented Aug 3, 2024

I'd ask when adding here: that's the purpose of the smaller overload for HashScanNoValues? It seems like we should just remove it and normalize on the one method, but maybe I'm missing a use case.

hey @NickCraver,, trying to remember my reasoning.. and after a quick look back to the code, you are most probably right.
I guess it was just the sake of keeping the convention and provide same method signatures with HashScan and HashScanAsync already there.
i dont have a strong opinion on it. What do you prefer ??

@NickCraver
Copy link
Collaborator

@atakavci Let's trim down and remove please - usually the #1 reason for overload is backwards compat, which isn't in play here :)

@atakavci
Copy link
Contributor Author

atakavci commented Aug 6, 2024

@atakavci Let's trim down and remove please - usually the #1 reason for overload is backwards compat, which isn't in play here :)

done 👍

@NickCraver NickCraver changed the title add support for hscan novalues option Add support for HSCAN NOVALUES Aug 18, 2024
@NickCraver NickCraver merged commit c0bb4eb into StackExchange:main Aug 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for HSCAN NOVALUES option
4 participants